Skip to content

fix(segmentation): apply modeConfiguration overrides#6051

Open
igoroctaviano wants to merge 2 commits into
OHIF:masterfrom
igoroctaviano:fix/segmentation-mode-configuration
Open

fix(segmentation): apply modeConfiguration overrides#6051
igoroctaviano wants to merge 2 commits into
OHIF:masterfrom
igoroctaviano:fix/segmentation-mode-configuration

Conversation

@igoroctaviano

@igoroctaviano igoroctaviano commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

The segmentation mode's modeFactory accepts a modeConfiguration argument but never applies it to the returned mode instance. As a result, any modesConfiguration['@ohif/mode-segmentation'] overrides from the app config (such as hide, displayName, routeName, etc.) are silently ignored for this mode.

Other modes (e.g. usAnnotation, tmtv, microscopy, preclinical-4d) already spread ...modeConfiguration into their returned object. This change brings the segmentation mode in line with them.

// platform/app/src/appInit.js
const modeConfiguration =
  appConfig.modesConfiguration && appConfig.modesConfiguration[id]
    ? appConfig.modesConfiguration[id]
    : {};
mode = await mode.modeFactory({ modeConfiguration, loadModules });

Before this fix, e.g. the following config had no effect on the segmentation mode:

modesConfiguration: {
  '@ohif/mode-segmentation': { hide: true },
},

Changes

  • Spread ...modeConfiguration into the object returned by the segmentation modeFactory.

Test plan

  • Add modesConfiguration: { '@ohif/mode-segmentation': { hide: true } } to the app config and confirm the Segmentation mode no longer appears in the mode selector.
  • Confirm overriding displayName for @ohif/mode-segmentation is reflected in the mode selector.
  • Confirm default behavior (no override) is unchanged.

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes
    • Fixed mode configuration to ensure all provided settings are properly applied when creating mode definitions.

Review Change Stack

Greptile Summary

This PR fixes a bug where the segmentation mode's modeFactory accepted a modeConfiguration argument but never spread it into the returned mode object, causing any app-config overrides (e.g. hide: true, custom displayName) to be silently ignored.

  • Adds ...modeConfiguration at the end of the returned object in modes/segmentation/src/index.tsx, matching the existing pattern used by every other OHIF mode.
  • Because the spread is the last entry in the object literal, caller-supplied overrides correctly take precedence over the defaults defined above them.

Confidence Score: 5/5

Safe to merge — a one-line fix that aligns the segmentation mode with the established pattern used by every other mode in the repo.

The change is a single spread operator added at the end of a plain object literal. It defaults to an empty object when no overrides are supplied, so existing behaviour is fully preserved. The same pattern is already in production across multiple other modes.

No files require special attention.

Important Files Changed

Filename Overview
modes/segmentation/src/index.tsx Adds ...modeConfiguration spread at the end of the object returned by modeFactory, consistent with all other OHIF modes, so app-config overrides (e.g. hide, displayName) are now applied.

Reviews (2): Last reviewed commit: "Merge branch 'master' into fix/segmentat..." | Re-trigger Greptile

The segmentation mode's modeFactory accepted a modeConfiguration
argument but never applied it to the returned mode instance, so
modesConfiguration overrides from the app config (e.g. hide,
displayName) were silently ignored for this mode. Spread
modeConfiguration into the returned object to match the other modes.

Co-authored-by: Cursor <cursoragent@cursor.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@netlify

netlify Bot commented May 29, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 8c82490
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a33eec6f36e820008eb0e6e
😎 Deploy Preview https://deploy-preview-6051--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The modeFactory function in the segmentation mode now propagates configuration options into the returned mode definition by spreading modeConfiguration into the object literal, ensuring configuration fields are included in the mode definition rather than discarded.

Changes

Mode Configuration Propagation

Layer / File(s) Summary
modeFactory configuration spread
modes/segmentation/src/index.tsx
modeFactory now spreads modeConfiguration into the returned mode object, merging any provided configuration fields into the mode definition.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A tiny thread of code is spun,
Configuration spread, now not undone,
The factory hops with glee so bright,
One little spread makes configs right! 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(segmentation): apply modeConfiguration overrides' clearly and specifically describes the main change—spreading modeConfiguration into the segmentation mode's returned object.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is comprehensive, well-structured, and addresses all major template sections with clear context, changes, and testing guidance.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@wayfarer3130 wayfarer3130 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually a better merge than ... will be needed, but it works ok for now to just have the direct over-ride of anything specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants